-
Notifications
You must be signed in to change notification settings - Fork 15
Remove CFFI in favor of native extension to support free-threaded mode #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #96 will degrade performances by 24.69%Comparing Summary
Benchmarks breakdown
|
Sounds good. I can't wait to see if this will pass on through or not. |
It seems that there is an issue when running In the runner we are setting the following: Do you know if there is any issue with free-threaded python and using |
@adriencaccia Sorry for my rather late response but there is one called |
Removed cffi package version 1.17.1 and its dependencies from the lock file.
33c1bfa to
411adc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates from CFFI-based Python bindings to native CPython extension module to enable free-threaded Python support. The change removes the CFFI dependency and replaces it with a hand-written C extension module that wraps the existing instrument hooks library.
Key changes:
- Removed CFFI dependency and replaced with native C extension API
- Added new C module
instrument_hooks_module.cimplementing Python bindings using CPython's C API - Updated
InstrumentHooksclass to use the new native module instead of CFFI'slibobject - Modified callgrind function calls to be methods on
InstrumentHooksinstead oflibproperties
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pytest_codspeed/instruments/valgrind.py | Updated callgrind instrumentation calls to use instance methods instead of lib object |
| src/pytest_codspeed/instruments/hooks/instrument_hooks_module.c | New native C extension module implementing Python bindings for instrument hooks |
| src/pytest_codspeed/instruments/hooks/build.py | Removed CFFI build configuration file |
| src/pytest_codspeed/instruments/hooks/init.py | Refactored to use native extension module and added callgrind methods |
| setup.py | Replaced CFFI extension with native Extension setup |
| pyproject.toml | Removed CFFI dependency and updated package data configuration |
| .github/workflows/ci.yml | Added Python 3.14t (free-threaded) to test matrix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int32_t pid; | ||
| const char *uri; | ||
|
|
||
| if (!PyArg_ParseTuple(args, "Oiy", &capsule, &pid, &uri)) { |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format string 'Oiy' is incorrect. The 'y' format expects const char* with length, but based on the C function signature instrument_hooks_set_executed_benchmark(hooks, pid, uri) which takes const char *uri, this should be 's' for a null-terminated string instead of 'y' which is for bytes with length.
| if (!PyArg_ParseTuple(args, "Oiy", &capsule, &pid, &uri)) { | |
| if (!PyArg_ParseTuple(args, "Ois", &capsule, &pid, &uri)) { |
| const char *name; | ||
| const char *version; | ||
|
|
||
| if (!PyArg_ParseTuple(args, "Oyy", &capsule, &name, &version)) { |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The format string 'Oyy' is incorrect. Similar to the previous issue, 'y' expects bytes with length. Based on the C function signature which takes const char* parameters, this should be 'Oss' for null-terminated strings.
| if (!PyArg_ParseTuple(args, "Oyy", &capsule, &name, &version)) { | |
| if (!PyArg_ParseTuple(args, "Oss", &capsule, &name, &version)) { |
| # Don't manually deinit - let the capsule destructor handle it | ||
| pass |
Copilot
AI
Nov 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The __del__ method with only a pass statement and comment should be removed entirely. An empty __del__ method can prevent proper garbage collection and the capsule destructor will be called automatically without defining __del__.
Fix #86